quiche: client session supports creating bidi stream#17543
quiche: client session supports creating bidi stream#17543alyssawilk merged 8 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM overall but I have some questions I want to sort out
| quic::QuicSpdyStream* | ||
| EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* /*pending*/) { | ||
| quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* pending) { | ||
| // Only client side server push stream should trigger this call. |
There was a problem hiding this comment.
Sorry I'm a bit confused - if the client side sends a server push stream when it's not negotiated why don't we close the connection for it doing "illegal" things? Should that be a TODO for quiche to fix upstream?
There was a problem hiding this comment.
Not just server push stream actually, instead, any client-initiated uni-directional stream with type byte other than the predesignated ones, QPack encode/decode streams, etc. QUICHE session should accept such stream as its extensions may wants to create such special streams. So it's not illegal for QUICHE.
I can close the connection in Envoy code if we don't have such use case in near future. Or update the comment to be more accurate?
There was a problem hiding this comment.
IIRC current QUIC support in Envoy is only for HTTP transactions which are always bi-directional. Closing the connections seems appropriate until other use cases are added.
There was a problem hiding this comment.
#17618 includes a QUICHE change that close the connection in this case. I will continue update this PR after that one is checked in.
| fmt::format( | ||
| "Quic session {} attempts to create stream {} before HCM filter is initialized.", | ||
| this->id(), pending->id())); | ||
| return nullptr; |
There was a problem hiding this comment.
Why not always return nullptr if we don't support?
Signed-off-by: Dan Zhang <danzh@google.com>
| static_cast<uint32_t>(GetReceiveWindow().value()), *filterManagerConnection(), | ||
| [this]() { runLowWatermarkCallbacks(); }, [this]() { runHighWatermarkCallbacks(); }, | ||
| stats, http3_options) {} | ||
|
|
There was a problem hiding this comment.
These changes seem unrelated. Was this code unused?
There was a problem hiding this comment.
These are only called by server push stream.
| quic::QuicSpdyStream* | ||
| EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* /*pending*/) { | ||
| quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* pending) { | ||
| // Only client side server push stream should trigger this call. |
There was a problem hiding this comment.
IIRC current QUIC support in Envoy is only for HTTP transactions which are always bi-directional. Closing the connections seems appropriate until other use cases are added.
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
Updated this PR using the newly sync'ed QUICHE flag to close connection upon receiving server push stream. PTAL! |
danzh2010
changed the title
* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ...
* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Though QUICHE won't negotiate to support server push, it doesn't prevent a problematic peer to open bi-directional streams. Risk: low Tests: new unit tess Docs: n/a release notes: n/a Signed-off-by: Dan Zhang <danzh@google.com>
4 participants
Though QUICHE won't negotiate to support server push, it doesn't prevent a problematic peer to open bi-directional streams. In such case, Envoy shouldn't hit NOT_REACHED_GCOVR_EXCL_LINE.